Fix AMP dev mode support #1143
Fix AMP dev mode support #1143
Conversation
felixarntz
requested review from
aaemnnosttv and
westonruter
and removed request for
westonruter
| 'script_loader_tag', | ||
| function ( $tag, $handle ) use ( $assets ) { | ||
| if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script ) { | ||
| // TODO: 'hoverintent-js' can be removed from here at some point, see https://github.com/ampproject/amp-wp/pull/3928. |
There was a problem hiding this comment.
Is this the right PR reference? Perhaps instead ampproject/amp-wp#4001?
There was a problem hiding this comment.
I think it is. ampproject/amp-wp#3928 is where you covered hoverintent-js so that it's properly flagged.
There was a problem hiding this comment.
Oh I see. It's handling cases where users are running older versions of the AMP plugin.
| $this->assets[ $handle ]->before_print(); | ||
|
|
||
| if ( $is_amp && $this->assets[ $handle ] instanceof Script ) { | ||
| $this->add_extra_script_amp_dev_mode( $handle ); |
There was a problem hiding this comment.
Note that this should no longer be necessary after ampproject/amp-wp#4001. As long as the dependency has the ampdevmode data added to it, there would be no more need to manually tag such inline scripts for matching a XPath expression.
There was a problem hiding this comment.
Thanks for reminding me of that! I've added two TODOs in the respective areas to remove our own fix in the future.
| function ( $tag, $handle ) use ( $assets ) { | ||
| if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script ) { | ||
| // TODO: 'hoverintent-js' can be removed from here at some point, see https://github.com/ampproject/amp-wp/pull/3928. | ||
| if ( $this->context->is_amp() && ( isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script || 'hoverintent-js' === $handle ) ) { |
There was a problem hiding this comment.
Wasn't the need to manually check for hoverintent-js made obsolete by ampproject/amp-wp#3928?
There was a problem hiding this comment.
Yes indeed (see above), however this was only in the quite recent 1.4.2 version. We'll keep this around for a bit more to have support for older AMP plugin versions. I'd be curious to see what the AMP plugin version distribution is :)
4 participants
Summary
Addresses issue #1142
Checklist